-
Notifications
You must be signed in to change notification settings - Fork 147
proxy: rest based light client #3782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
6a42166 to
10b6757
Compare
| stopAtEpoch: 0, | ||
| ) | ||
| proc completeCmdArg*(T: type UrlList, val: string): seq[string] = | ||
| return @[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need return here
| getBeaconTime: getBeaconTime, | ||
| ) | ||
|
|
||
| # https://github.com/ethereum/consensus-specs/blob/v1.6.0-alpha.3/specs/altair/light-client/p2p-interface.md#getlightclientbootstrap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these URLs had been just copy/pasted from somewhere else, sure, at least within this PR this (and the other v1.6.0 alpha.3 references, and v1.4.0-beta.5, and etc) seem to arise from nowhere existing?
If they're being added newly, then better to ensure they're consistent with the current v1.6.0-beta.1.
| static: | ||
| doAssert false | ||
|
|
||
| iterator values(v: auto): auto = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as one is careful not to export this -- values with an auto type will otherwise cause potential issues with things importing this module.
| # reattempt bootstrap download in 2 seconds | ||
| if trustedBlockRoot.isNone: | ||
| debug "TrustedBlockRoot unavaialble re-attempting bootstrap download" | ||
| await sleepAsync(chronos.seconds(2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these different 2 second sleeps be consistent with each other, or are the both incidentally 2 seconds? If they should be consistent with each other, then better to extract it as a constant and use that constant in both, so that if one is changed, the other is automatically changed.
| let didProgress = await self.query(OptimisticUpdate) | ||
| if not didProgress: | ||
| # retry in 2 seconds | ||
| await sleepAsync(chronos.seconds(2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these 2 second retry timers be intrinsically consistent?
| let | ||
| wallTime = self.getBeaconTime() | ||
| currentSlot = wallTime.slotOrZero() | ||
| currentEpoch = (currentSlot mod SLOTS_PER_EPOCH) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CL has a epoch helper for this.
| finalizedEpoch = (finalizedSlot mod SLOTS_PER_EPOCH) | ||
| optimisticSlot = self.getOptimisticSlot() | ||
| optimisticPeriod = optimisticSlot.sync_committee_period | ||
| optimisitcEpoch = (optimisticSlot mod SLOTS_PER_EPOCH) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optimisitcEpoch has a typo, but more importantly, should be able to use optimisticSlot.epoch
| currentPeriod = currentSlot.sync_committee_period | ||
| finalizedSlot = self.getFinalizedSlot() | ||
| finalizedPeriod = finalizedSlot.sync_committee_period | ||
| finalizedEpoch = (finalizedSlot mod SLOTS_PER_EPOCH) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
finalizedSlot.epoch
| if not self.isLightClientStoreInitialized(): | ||
| let trustedBlockRoot = self.getTrustedBlockRoot() | ||
|
|
||
| # reattempt bootstrap download in 2 seconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general point, how does this scale with different slot times? Should these 2 second timers be the same with 5, 6, or 12 second slots or should they scale?
| reqId: uint64, blockRoot: Eth2Digest | ||
| ): Future[NetRes[ForkedLightClientBootstrap]] {.async: (raises: [CancelledError]).} = | ||
| let | ||
| peer = client.peers[reqId mod uint64(client.peers.len)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CL has a helper, lenu64, maybe available here
| reqId: uint64, startPeriod: SyncCommitteePeriod, count: uint64 | ||
| ): Future[LightClientUpdatesByRangeResponse] {.async: (raises: [CancelledError]).} = | ||
| let | ||
| peer = client.peers[reqId mod uint64(client.peers.len)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if lenu64 is available, this is exactly what it's for
| if ctx != nil and ctx.stop: | ||
| # Cleanup | ||
| waitFor network.stop() | ||
| waitFor lcRestClient.closeAll() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are going to be multiple waitFors in this function, is it possible to make the function itself async, use await, and have only one waitFor on the outside?
| beacon_chain/conf_light_client, | ||
| beacon_chain/nimbus_binary_common | ||
| beacon_chain/nimbus_binary_common, | ||
| std/strutils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually put stdlib imports first
| # (Untrusted) web3 provider | ||
| # No default - Needs to be provided by the user | ||
| lcEndpoints* {. | ||
| desc: "command seperated URLs of the light client data provider", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separated
| normalizedScheme = parsed.scheme.toLowerAscii() | ||
|
|
||
| if not (normalizedScheme == "http" or normalizedScheme == "https"): | ||
| raise newException(ValueError, "Light Client Endpoint should be a http(s) url") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URL is an acronym and capitalized as such
meta: #3101
Description:
This PR ports the existing light client within nimbus-eth2 for use in the verified proxy with light client updates downloaded purely via the REST API. This is beneficial for using the proxy in a browser. In doing so, the PR modifies the behaviour of the light client slightly as noted below.
Notes:
Syncing: The original light client starts with a bootstrap and syncs blocks from bootstrap to head. It then follows the chain optimistically. here and here. The verified proxy version of the light client uses the bootstrap update and subsequent sync committee updates only for establishing the (current and next) sync committee. Once established, however, it starts following the chain directly from the current head. From the perspective of trust, optimistically following the chain from the current head is the same as syncing the chain and then following it optimistically (since the validation is only dependent on the current sync committee which doesn't change within the current sync committee period) here
Peers and Endpoints: The original light client uses multiple peers concurrently to download a particular update. The verified proxy version uses the same code but abstracts out rotating/randomizing peers (in the case of NVP it is API endpoints) to the backend implementation through the use of
reqIds. The modified light client uses unique request ids for all concurrent requests which can then be used to rotate/randomize and score API endpoints. The parallelization parameter for the light client is hardcoded to 2.Downloading Updates: The original light client used a mix of light client updates and p2p gossip to maintain the canonical chain. The verified proxy version purely uses light client updates to maintain the canonical chain. Hence, it runs the manager loop on a per slot frequency rather than the per sync committee period frequency of the original client(not fully true, it is quite arbitrary but can be considered the average case scenario)
Other Changes:
FullVersionStr)